Skip to content

transport: Avoid buffer copies when reading Data frames #8657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Oct 31, 2025
Merged

transport: Avoid buffer copies when reading Data frames #8657

merged 12 commits into from
Oct 31, 2025

Conversation

@arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Oct 16, 2025 •
edited
Loading

This change incorporates changes from golang/go#73560 to split reading HTTP/2 frame headers and payloads. If the frame is not a Data frame, it's read through the standard library framer as before. For Data frames, the payload is read directly into a buffer from the buffer pool to avoid copying it from the framer's buffer.

Testing

For 1 MB payloads, this results in ~4% improvement in throughput.

# test command
go run benchmark/benchmain/main.go -benchtime=60s -workloads=streaming \
   -compression=off -maxConcurrentCalls=120 -trace=off \
   -reqSizeBytes=1000000 -respSizeBytes=1000000 -networkMode=Local -resultFile="${RUN_NAME}"

# comparison
go run benchmark/benchresult/main.go streaming-before streaming-after  
               Title       Before        After Percentage
            TotalOps        87536        91120     4.09%
             SendOps            0            0      NaN%
             RecvOps            0            0      NaN%
            Bytes/op   4074102.92   4070489.30    -0.09%
           Allocs/op        83.60        76.55    -8.37%
             ReqT/op 11671466666.67 12149333333.33     4.09%
            RespT/op 11671466666.67 12149333333.33     4.09%
            50th-Lat  78.209875ms  75.159943ms    -3.90%
            90th-Lat 117.764228ms   107.8697ms    -8.40%
            99th-Lat 146.935704ms 139.069685ms    -5.35%
             Avg-Lat  82.310691ms  79.073282ms    -3.93%
           GoVersion     go1.24.7     go1.24.7
         GrpcVersion   1.77.0-dev   1.77.0-dev

For smaller payloads, the difference in minor.

go run benchmark/benchmain/main.go -benchtime=60s -workloads=streaming \
   -compression=off -maxConcurrentCalls=120 -trace=off \
   -reqSizeBytes=100 -respSizeBytes=100 -networkMode=Local -resultFile="${RUN_NAME}"

go run benchmark/benchresult/main.go streaming-before streaming-after 
               Title       Before        After Percentage
            TotalOps     21490752     21477822    -0.06%
             SendOps            0            0      NaN%
             RecvOps            0            0      NaN%
            Bytes/op      1902.92      1902.94     0.00%
           Allocs/op        29.21        29.21     0.00%
             ReqT/op 286543360.00 286370960.00    -0.06%
            RespT/op 286543360.00 286370960.00    -0.06%
            50th-Lat    352.505µs    352.247µs    -0.07%
            90th-Lat    433.446µs    434.907µs     0.34%
            99th-Lat    536.445µs    539.759µs     0.62%
             Avg-Lat    333.403µs    333.457µs     0.02%
           GoVersion     go1.24.7     go1.24.7
         GrpcVersion   1.77.0-dev   1.77.0-dev

RELEASE NOTES:

  • transport: Avoid a buffer copy when reading data.

@arjan-bal arjan-bal added this to the 1.77 Release milestone Oct 16, 2025
@arjan-bal arjan-bal added Type: Performance Performance improvements (CPU, network, memory, etc) Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels Oct 16, 2025
@codecov
Copy link

codecov bot commented Oct 16, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 85.88235% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.29%. Comparing base (64cba2d) to head (f0e9b13).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
internal/transport/http_util.go 84.37% 3 Missing and 7 partials ⚠️
internal/transport/http2_client.go 90.90% 1 Missing ⚠️
internal/transport/http2_server.go 90.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8657      +/-   ##
==========================================
- Coverage   83.29%   83.29%   -0.01%     
==========================================
  Files         415      415              
  Lines       32134    32196      +62     
==========================================
+ Hits        26767    26817      +50     
- Misses       4001     4005       +4     
- Partials     1366     1374       +8
Files with missing lines Coverage Δ
mem/buffer_pool.go 100.00% <ø> (ø)
internal/transport/http2_client.go 92.90% <90.90%> (+0.19%) ⬆️
internal/transport/http2_server.go 91.07% <90.00%> (ø)
internal/transport/http_util.go 90.77% <84.37%> (-4.45%) ⬇️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arjan-bal arjan-bal force-pushed the copyless-data-frame-read branch from b5f777e to ae8c8df Compare October 16, 2025 16:54
@arjan-bal arjan-bal self-assigned this Oct 16, 2025
@arjan-bal arjan-bal force-pushed the copyless-data-frame-read branch 2 times, most recently from efe661b to 778af0a Compare October 16, 2025 18:53
@arjan-bal arjan-bal force-pushed the copyless-data-frame-read branch from 778af0a to 45e856f Compare October 16, 2025 19:12
@arjan-bal arjan-bal force-pushed the copyless-data-frame-read branch from 45e856f to 5ec4a4e Compare October 16, 2025 19:15
@arjan-bal arjan-bal removed their assignment Oct 16, 2025
@arjan-bal arjan-bal requested review from dfawley and easwars and removed request for easwars October 16, 2025 19:21
easwars
easwars reviewed Oct 16, 2025
View reviewed changes
@easwars easwars assigned arjan-bal and unassigned easwars Oct 16, 2025
arjan-bal
arjan-bal commented Oct 21, 2025
View reviewed changes
Copy link
Contributor Author

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realised that the changes in the std lib framer are not imported in g3 os this PR can't be merged in gRPC yet. I've added a Blocked label to indicate this.

@arjan-bal arjan-bal removed their assignment Oct 21, 2025
@easwars
Copy link
Contributor

easwars commented Oct 27, 2025

I realised that the changes in the std lib framer are not imported in g3 os this PR can't be merged in gRPC yet. I've added a Blocked label to indicate this.

When is it expected to be imported?

easwars
easwars reviewed Oct 27, 2025
View reviewed changes
@easwars easwars assigned arjan-bal and unassigned dfawley Oct 27, 2025
@arjan-bal
Copy link
Contributor Author

I realised that the changes in the std lib framer are not imported in g3 os this PR can't be merged in gRPC yet. I've added a Blocked label to indicate this.

When is it expected to be imported?

I think importing the code is self-service. I'm working on it: cl/824893868

@easwars easwars assigned dfawley and unassigned easwars and arjan-bal Oct 28, 2025
@easwars
Copy link
Contributor

easwars commented Oct 28, 2025

@dfawley: Moving to your plate in case you want to take a look. Thanks.

@arjan-bal arjan-bal modified the milestones: 1.77 Release, 1.78 Release Oct 30, 2025
dfawley
dfawley approved these changes Oct 30, 2025
View reviewed changes
internal/transport/http2_client.go
t.controlBuf.put(&outgoingWindowUpdate{s.id, w})
}
}
// TODO(bradfitz, zhaoq): A copy is required here because there is no
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

internal/transport/http_util.go
Comment on lines +413 to +417
if memPool == nil {
// Note that this is only supposed to be nil in tests. Otherwise, stream
// is always initialized with a BufferPool.
memPool = mem.DefaultBufferPool()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it hard to fix the tests? Maybe we can do that as a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised #8688 with the cleanup.

@dfawley dfawley assigned arjan-bal and unassigned dfawley Oct 30, 2025
arjan-bal and others added 2 commits October 31, 2025 11:13
@arjan-bal @dfawley
Apply suggestions from code review
a121b15 
Co-authored-by: Doug Fawley <dfawley@google.com>
@arjan-bal
Fix comment
e7b6d35
@arjan-bal arjan-bal force-pushed the copyless-data-frame-read branch from 8537f97 to e7b6d35 Compare October 31, 2025 06:08
@arjan-bal arjan-bal merged commit 363018c into grpc:master Oct 31, 2025
14 checks passed
@arjan-bal arjan-bal deleted the copyless-data-frame-read branch October 31, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@easwars easwars easwars approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@arjan-bal arjan-bal

Labels

Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. Type: Performance Performance improvements (CPU, network, memory, etc)

Projects

None yet

Milestone

1.77 Release

Development

Successfully merging this pull request may close these issues.

3 participants

@arjan-bal @easwars @dfawley